e2e/global-setup.ts (2)

48-49: ⚡ Quick win

Misleading comment.

The comment states "Verify at least one bundle ID got the settings applied" but no verification code follows. The subsequent lines (52-63) apply additional settings to system-level domains (LaunchServices, CrashReporter) rather than verifying the bundle-specific settings.

📝 Clarify the comment

-        // Verify at least one bundle ID got the settings applied.
-        // Also suppress the "verification of developer" dialog that can appear
+        // Suppress the "verification of developer" dialog that can appear

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@e2e/global-setup.ts` around lines 48 - 49, The comment claiming "Verify at
least one bundle ID got the settings applied" is misleading — update the comment
above the subsequent settings block to accurately describe what the code does
(apply additional system-level settings and suppress the "verification of
developer" dialog) and remove the mention of bundle-ID verification; reference
the system domains mentioned (LaunchServices, CrashReporter) in the new comment
so it correctly documents that the following lines adjust system-level domains
rather than performing any bundle-specific verification.

---

30-64: 💤 Low value

Consider using execFileSync for security.

The current implementation uses execSync with string interpolation of bundleID. While the current code uses a hard-coded array, using execFileSync with argument arrays would be more robust against potential future changes and aligns with security best practices.

🔒 Proposed refactor using execFileSync

-import {execSync} from 'child_process';
+import {execFileSync} from 'child_process';

         for (const bundleID of bundleIDs) {
             try {
-                execSync(`defaults write ${bundleID} NSQuitAlwaysKeepsWindows -bool false`, {stdio: 'pipe'});
+                execFileSync('defaults', ['write', bundleID, 'NSQuitAlwaysKeepsWindows', '-bool', 'false'], {stdio: 'pipe'});
             } catch {
                 // Non-fatal — tests still run, just potentially with the Resume dialog
             }
             try {
-                execSync(`defaults write ${bundleID} ApplePersistenceIgnoreState -bool YES`, {stdio: 'pipe'});
+                execFileSync('defaults', ['write', bundleID, 'ApplePersistenceIgnoreState', '-bool', 'YES'], {stdio: 'pipe'});
             } catch {
                 // Non-fatal
             }
         }
 
         try {
-            execSync('defaults write com.apple.LaunchServices LSQuarantine -bool false', {stdio: 'pipe'});
+            execFileSync('defaults', ['write', 'com.apple.LaunchServices', 'LSQuarantine', '-bool', 'false'], {stdio: 'pipe'});
         } catch {
             // Non-fatal
         }
 
         try {
-            execSync('defaults write com.apple.CrashReporter DialogType none', {stdio: 'pipe'});
+            execFileSync('defaults', ['write', 'com.apple.CrashReporter', 'DialogType', 'none'], {stdio: 'pipe'});
         } catch {
             // Non-fatal
         }

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@e2e/global-setup.ts` around lines 30 - 64, Replace the execSync calls in the
macOS branch (process.platform === 'darwin') that interpolate bundleID into
command strings with execFileSync using argument arrays: locate the loop over
bundleIDs and the standalone execSync calls (symbols: bundleIDs, execSync) and
call execFileSync('defaults', ['write', bundleID, 'NSQuitAlwaysKeepsWindows',
'-bool', 'false'], {stdio:'pipe'}) and similarly for the other keys
(ApplePersistenceIgnoreState, LSQuarantine, DialogType) to avoid shell
interpolation; preserve the existing try/catch non-fatal behavior and same stdio
option.

.github/workflows/e2e-functional-template.yml (1)

395-410: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Use the standard release bucket and CDN URL for Playwright reports.

This still hardcodes the backing bucket and emits a direct S3 URL. The repo workflow convention is to upload via MM_DESKTOP_RELEASE_BUCKET and publish releases.mattermost.com/desktop/... links instead.

💡 Suggested adjustment

-          AWS_S3_BUCKET: mattermost-cypress-report
+          AWS_S3_BUCKET: ${{ secrets.MM_DESKTOP_RELEASE_BUCKET }}
...
-          REPORT_URL="https://${AWS_S3_BUCKET}.s3.amazonaws.com/${S3_PREFIX}/index.html"
+          REPORT_URL="https://releases.mattermost.com/desktop/${S3_PREFIX}/index.html"

Based on learnings: "Artifact uploads in Mattermost Desktop workflows should funnel to a backend S3 bucket defined by the MM_DESKTOP_RELEASE_BUCKET secret, but the public URL for accessing those artifacts must always be releases.mattermost.com/desktop via the CDN/mapping layer."

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In @.github/workflows/e2e-functional-template.yml around lines 395 - 410,
Replace the hardcoded AWS_S3_BUCKET and direct S3 REPORT_URL with the
repo-standard secret and CDN URL: set AWS_S3_BUCKET to the secret
MM_DESKTOP_RELEASE_BUCKET (instead of
secrets.MM_DESKTOP_E2E_AWS_SECRET_ACCESS_KEY or the current literal), keep
S3_PREFIX as-is, use that bucket in the aws s3 sync target, and construct
REPORT_URL using the CDN mapping
"https://releases.mattermost.com/desktop/${S3_PREFIX}/index.html" so uploads go
to the canonical release bucket and public links use
releases.mattermost.com/desktop.

src/main/UserActivityMonitor.test.js (1)

6-15: ⚡ Quick win

Use the repo-standard ESM singleton mock shape.

This electron mock omits __esModule: true and default, so it diverges from the required *.test.js singleton pattern and can cause import interop drift across the suite.

♻️ Suggested adjustment

+const electronMock = {
+    app: {
+        isReady: jest.fn(() => true),
+    },
+    powerMonitor: {
+        getSystemIdleTime: jest.fn(() => 0),
+    },
+};
+
-jest.mock('electron', () => ({
-    app: {
-        isReady: jest.fn(() => true),
-    },
-    powerMonitor: {
-        getSystemIdleTime: jest.fn(() => 0),
-    },
-}));
+jest.mock('electron', () => ({
+    __esModule: true,
+    default: electronMock,
+    ...electronMock,
+}));

As per coding guidelines: "**/*.test.{js,ts}: Mock singletons with jest.mock() using __esModule: true + default property, and use jest.mocked() for type-safe access to mock functions`."

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/main/UserActivityMonitor.test.js` around lines 6 - 15, The electron mock
is missing the repo-standard ESM singleton shape; update the
jest.mock('electron', ...) to return { __esModule: true, default: { app: {
isReady: jest.fn(() => true) }, powerMonitor: { getSystemIdleTime: jest.fn(() =>
0) } } } so imports get the default ESM singleton and the mocked functions
(app.isReady and powerMonitor.getSystemIdleTime) behave correctly; after
changing the mock shape, use jest.mocked(...) when referencing these mocks
elsewhere in the test to keep type-safe access.

src/main/diagnostics/index.test.js (1)

6-30: ⚡ Quick win

Use the repo-standard ESM singleton mock shape here too.

This new electron mock has the same issue: it skips the required __esModule: true + default wrapper for singleton mocks, so it does not match the repo's Jest mock contract.

As per coding guidelines: "**/*.test.{js,ts}: Mock singletons with jest.mock() using __esModule: true + default property, and use jest.mocked() for type-safe access to mock functions`."

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/main/diagnostics/index.test.js` around lines 6 - 30, The electron
jest.mock should use the repo-standard ESM singleton shape by returning an
object with __esModule: true and a default property containing the mocked API;
update the existing jest.mock('electron', ...) to wrap the current mock
(shell.showItemInFolder, app.isReady/getPath, powerMonitor.getSystemIdleTime,
session.defaultSession, systemPreferences.getMediaAccessStatus, Notification)
under a default key and add __esModule: true so the singleton mock contract is
satisfied; after changing the mock shape, use jest.mocked(...) where this mock
is referenced in tests to get type-safe access to the mocked functions.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes